refactor: consolidate duplicated model-normalization helpers (pkg/cli ↔ pkg/modelsdev)#43340
Conversation
… ↔ pkg/modelsdev) - Export NormalizeProvider and NormalizeComparableModelID from pkg/modelsdev/catalog.go - Remove byte-for-byte duplicate normalizeCatalogProvider / normalizeComparableModelID from pkg/cli/model_costs.go and replace all call sites with the modelsdev versions - Add NormalizeProvider / NormalizeComparableModelID unit tests in catalog_test.go - Update TestNormalizeCatalogProvider in model_costs_test.go to use modelsdev.NormalizeProvider Closes #43270 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43340 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (53 additions detected, threshold is 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR removes duplicated provider/model normalization helpers by exporting the canonical implementations from pkg/modelsdev and reusing them from pkg/cli, reducing the risk of silent logic divergence across packages.
Changes:
- Export
NormalizeProviderandNormalizeComparableModelIDfrompkg/modelsdevand update internal call sites. - Replace the duplicated helpers in
pkg/cli/model_costs.gowith calls to the exportedmodelsdevfunctions. - Add/adjust unit tests to cover the exported normalizers and update CLI tests accordingly.
Show a summary per file
| File | Description |
|---|---|
| pkg/modelsdev/catalog.go | Exports the provider/model normalization helpers and updates catalog lookup/parse call sites to use them. |
| pkg/modelsdev/catalog_test.go | Adds direct unit coverage for the newly exported normalization helpers. |
| pkg/cli/model_costs.go | Removes local duplicate helpers and uses modelsdev.NormalizeProvider / NormalizeComparableModelID throughout. |
| pkg/cli/model_costs_test.go | Updates the CLI normalization test to call modelsdev.NormalizeProvider (and adds the new import). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/modelsdev/catalog.go:223
- NormalizeProvider currently re-computes strings.ToLower(strings.TrimSpace(provider)) in both the switch expression and the default branch. Computing it once improves readability and avoids duplicated work.
switch strings.ToLower(strings.TrimSpace(provider)) {
case "github", "copilot", "github_models":
return "github-copilot"
default:
return strings.ToLower(strings.TrimSpace(provider))
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
| t.Run(name, func(t *testing.T) { | ||
| got := normalizeCatalogProvider(tt.input) | ||
| got := modelsdev.NormalizeProvider(tt.input) | ||
| assert.Equal(t, tt.want, got) |
| // NormalizeComparableModelID lower-cases the value and replaces "." and "_" with "-" | ||
| // so that model IDs differing only in those separators compare equal. | ||
| func NormalizeComparableModelID(value string) string { | ||
| return strings.NewReplacer(".", "-", "_", "-").Replace(strings.ToLower(strings.TrimSpace(value))) | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: refactor — consolidate duplicated model-normalization helpers
This is a clean, well-scoped refactor. The duplication hazard is real (silent divergence between pkg/cli and pkg/modelsdev), and the fix is correct.
What looks good
- Both helpers are promoted with doc comments and thoroughly tested in
catalog_test.go. - All six call sites in
model_costs.goare updated consistently; no leftover references to the old local copies. pkg/clialready importedpkg/modelsdev, so no new import cycle is introduced.- Test coverage added for both exported functions covers aliases, case-folding, trimming, and separator normalization.
One observation (not blocking)
pkg/workflow/compiler_model_pricing.go still contains a third copy of the same logic (normalizeProviderForPricing, ~line 81), with identical switch cases. That package does not currently import pkg/modelsdev, so this PR correctly left it alone. A follow-up to consolidate that copy (if no circular import exists) would complete the deduplication.
Verdict: No blocking issues. Approving.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 28.9 AIC · ⌖ 5.84 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (3 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — no blocking issues found; leaving three minor suggestions.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Excellent single-responsibility refactor: two byte-for-byte duplicates eliminated, single canonical implementation in the right package
- ✅ Six call sites updated consistently with no behavioural change
- ✅ Good doc comments added to both exported functions — they now form a proper public API
- ✅ Comprehensive new tests in
catalog_test.gocovering aliases, case-folding, trimming, and separator normalisation
Minor Suggestions
-
Stale test name (
pkg/cli/model_costs_test.goline 45) —TestNormalizeCatalogProvidernow testsmodelsdev.NormalizeProvider; the name is misleading and the cases duplicateTestNormalizeProviderincatalog_test.go. Either remove it or replace with a wiring-level integration test forfindModelPricing. -
Missing mixed-case alias (
pkg/modelsdev/catalog_test.goline 156) —"GitHub_Models"(mixed caps) is not in theTestNormalizeProvidertable. Adding it locks in the case-insensitivity invariant as a specification. -
Missing idempotency case (
pkg/modelsdev/catalog_test.goline 178) —TestNormalizeComparableModelIDhas no case for already-canonical input. A{"gpt-4o", "gpt-4o"}row guards against future over-normalisation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 60.8 AIC · ⌖ 6.86 AIC · ⊞ 6.6K
Comment /matt to run again
| } | ||
| t.Run(name, func(t *testing.T) { | ||
| got := normalizeCatalogProvider(tt.input) | ||
| got := modelsdev.NormalizeProvider(tt.input) |
There was a problem hiding this comment.
[/codebase-design] TestNormalizeCatalogProvider now tests modelsdev.NormalizeProvider, making its name stale — and its cases are a subset of the new TestNormalizeProvider in catalog_test.go, so the coverage is duplicated.
💡 Suggested fix
Either remove this test entirely (canonical coverage already lives in pkg/modelsdev/catalog_test.go), or rename it and replace the cases with an integration-level test that goes through findModelPricing with a provider alias to verify the wiring end-to-end. Keeping it as-is means a failure reports TestNormalizeCatalogProvider when the function named in the message is NormalizeProvider, which is confusing.
@copilot please address this.
| {"copilot", "github-copilot"}, | ||
| {"github_models", "github-copilot"}, | ||
| {"GITHUB_MODELS", "github-copilot"}, | ||
| {"anthropic", "anthropic"}, |
There was a problem hiding this comment.
[/tdd] TestNormalizeProvider has no case for "github_models" with mixed capitalisation (e.g. "GitHub_Models"). The switch only lower-cases before comparing, so capitalised variants work, but a test case would lock that in as a specification.
💡 Suggested additional case
{"GitHub_Models", "github-copilot"},Adding it turns the test suite into a living specification for alias matching, and prevents a future refactor from accidentally dropping case-insensitive handling.
@copilot please address this.
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.input+"->"+tc.want, func(t *testing.T) { | ||
| assert.Equal(t, tc.want, NormalizeComparableModelID(tc.input)) |
There was a problem hiding this comment.
[/tdd] TestNormalizeComparableModelID is missing a case for a string that is already in canonical form (e.g. "gpt-4o" → "gpt-4o"). Without an idempotency case, a future change that over-normalises (e.g. double-stripping) would not be caught by this suite.
💡 Suggested additional case
{"gpt-4o", "gpt-4o"}, // already canonical — idempotency check@copilot please address this.
There was a problem hiding this comment.
Non-blocking observation
The refactor is well-scoped: the consolidation is correct, all six call sites are updated, and the new test coverage is solid.
One minor test-hygiene issue: the empty-input sub-test name (see inline comment).
🔎 Code quality review by PR Code Quality Reviewer · 151.4 AIC · ⌖ 8.85 AIC · ⊞ 5.4K
Comment /review to run again
| {"", ""}, | ||
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.input+"->"+tc.want, func(t *testing.T) { |
There was a problem hiding this comment.
Unaddressable sub-test name: when tc.input is "" the sub-test name becomes "->", which cannot be targeted with -run and silently conflates with any other "->" case in test output.
💡 Suggested fix
Apply the same guard used in the CLI test (model_costs_test.go:41):
for _, tc := range cases {
name := tc.input + "->" + tc.want
if tc.input == "" {
name = "<empty>->" + tc.want
}
t.Run(name, func(t *testing.T) {
assert.Equal(t, tc.want, NormalizeProvider(tc.input))
})
}Without this, go test -run TestNormalizeProvider/-> silently matches nothing, and CI logs show "->" with no way to identify which empty case failed. The same issue exists in TestNormalizeComparableModelID (line 177).
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Both review items addressed in commit 3c3bab3:
Local validation ( |
…normalizes) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
pelikhan
deleted the
copilot/refactor-consolidate-model-normalization-helpers
branch
pkg/cli/model_costs.gocontained byte-for-byte copies of two unexported helpers frompkg/modelsdev/catalog.go— a silent divergence hazard for provider-alias and model-ID normalization logic.Changes
pkg/modelsdev/catalog.go— promotenormalizeProvider→NormalizeProviderandnormalizeComparableModelID→NormalizeComparableModelID(exported, with doc comments); update all internal call sitespkg/cli/model_costs.go— delete localnormalizeCatalogProviderandnormalizeComparableModelID; replace all six call sites withmodelsdev.NormalizeProvider/modelsdev.NormalizeComparableModelID(no new import — file already depended onpkg/modelsdev)pkg/modelsdev/catalog_test.go— addTestNormalizeProviderandTestNormalizeComparableModelIDcovering aliases, case-folding, trimming, and separator normalizationpkg/cli/model_costs_test.go— updateTestNormalizeCatalogProviderto callmodelsdev.NormalizeProvider